Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @liorgold2 and @Yoni-Starkware)

crates/apollo_starknet_os_program/src/cairo/starkware/starknet/core/os/naive_blake.cairo line 25 at r1 (raw file):

    const MESSAGE_SHIFT = 2 ** 32;
    const FLAGS_SHIFT = 2 ** 48;
    const OPCODE_EXT_SHIFT = 2 ** 63;

Not sure if these are needed, we can just write the numbers in the final constant, just added them to be consistent with the type rs implementation

Code quote:

    const COUNTER_SHIFT = 1;
    const STATE_SHIFT = 2 ** 16;
    const MESSAGE_SHIFT = 2 ** 32;
    const FLAGS_SHIFT = 2 ** 48;
    const OPCODE_EXT_SHIFT = 2 ** 63;

crates/apollo_starknet_os_program/src/cairo/starkware/starknet/core/os/naive_blake.cairo line 29 at r1 (raw file):

    const POS_STATE_OFFSET = 2 ** 15 + STATE_OFFSET;
    const POS_MESSAGE_OFFSET = 2 ** 15 + MESSAGE_OFFSET;
    const POS_COUNTER_OFFSET = 2 ** 15 + COUNTER_OFFSET;

Added this content because the format fix did not handle the parentheses well in the BLAKE2S_FINALIZE_INSTRUCTION constant

Code quote:

    const POS_STATE_OFFSET = 2 ** 15 + STATE_OFFSET;
    const POS_MESSAGE_OFFSET = 2 ** 15 + MESSAGE_OFFSET;
    const POS_COUNTER_OFFSET = 2 ** 15 + COUNTER_OFFSET;

@liorgold2 reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @meship-starkware and @Yoni-Starkware)

crates/apollo_starknet_os_program/src/cairo/starkware/starknet/core/os/naive_blake.cairo line 25 at r1 (raw file):

Good idea, let's remove these constants.

crates/apollo_starknet_os_program/src/cairo/starkware/starknet/core/os/naive_blake.cairo line 29 at r1 (raw file):

Ok.

crates/apollo_starknet_os_program/src/cairo/starkware/starknet/core/os/naive_blake.cairo line 14 at r1 (raw file):

    // - State and data are referenced from fp
    // - Counter is referenced from ap
    // - Increment ap by 1 after instruction

Suggestion:

    // Flags: 000100000001010.
    // - State and data are referenced from fp.
    // - Counter is referenced from ap.
    // - Increment ap by 1 after the instruction.

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @meship-starkware and @Yoni-Starkware)

crates/apollo_starknet_os_program/src/cairo/starkware/starknet/core/os/naive_blake.cairo line 14 at r1 (raw file):

    // - State and data are referenced from fp
    // - Counter is referenced from ap
    // - Increment ap by 1 after instruction

Actually, you can comment on the flags:

    const OP0_REG = 1;  // State is fp-based.
    const OP1_FP = 3;  // Data is fp-based.
    const AP_ADD1 = 11;  // Increment ap by 1 after the instruction.

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @liorgold2 and @Yoni-Starkware)

crates/apollo_starknet_os_program/src/cairo/starkware/starknet/core/os/naive_blake.cairo line 14 at r1 (raw file):

Done.

@liorgold2 reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @meship-starkware and @Yoni-Starkware)

crates/apollo_starknet_os_program/src/cairo/starkware/starknet/core/os/naive_blake.cairo line 14 at r1 (raw file):

See second reply - move comments to the constants.

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @meship-starkware)

crates/apollo_starknet_os_program/src/cairo/starkware/starknet/core/os/naive_blake.cairo line 6 at r2 (raw file):

// Computes blake2s of `input` of size 16 felts, representing 32 bits each.
// The initial state is the standard BLAKE2s IV XORed with the parameter block P[0] = 0x01010020.
func blake_with_opcode_for_single_16_length_word(data: felt*, out: felt*, initial_state: felt*) {

@meship-starkware I wonder if adding noise to this function (+-1 to one of the consts and removing the static asserts) makes some tests fail. In particular, some consistency test with Rust.

Reviewable status: 1 of 2 files reviewed, 2 unresolved discussions (waiting on @liorgold2)

crates/apollo_starknet_os_program/src/cairo/starkware/starknet/core/os/naive_blake.cairo line 14 at r1 (raw file):

Done.

Reviewable status: 1 of 2 files reviewed, 2 unresolved discussions (waiting on @liorgold2 and @Yoni-Starkware)

crates/apollo_starknet_os_program/src/cairo/starkware/starknet/core/os/naive_blake.cairo line 6 at r2 (raw file):

Changing one of the constants would fail the test that checks the encryption, as the Blake opcode will return something else. Removing the static asserts won't fail anything, but can we make a test that will fail if someone removes the static assert? If someone removes the static assert, it will change the order of the function arguments; the Blake opcode would fail as well, it would just be a bit harder to understand the failure reason

@ArniStarkware This Solve the merge conflict with main

Reviewable status: 1 of 2 files reviewed, 2 unresolved discussions (waiting on @liorgold2 and @Yoni-Starkware)

Reviewable status: 1 of 2 files reviewed, 2 unresolved discussions (waiting on @liorgold2 and @meship-starkware)

crates/apollo_starknet_os_program/src/cairo/starkware/starknet/core/os/naive_blake.cairo line 6 at r2 (raw file):

Static asserts are sanity check. I told you to remove them because I didn't want them to catch your noise.
What about AP_ADD1?

Reviewable status: 1 of 2 files reviewed, 2 unresolved discussions (waiting on @liorgold2 and @Yoni-Starkware)

crates/apollo_starknet_os_program/src/cairo/starkware/starknet/core/os/naive_blake.cairo line 6 at r2 (raw file):

It seems that in the Cairo common function, we are changing the range_check value, but I'm not sure what the reason is. This requires incrementing ap by one. I tried to mimic this behavior, and now the test fails without this flag. Do we need this range_check check in our function as well
https://github.com/starkware-industries/starkware/blob/930126a61d0b9352591283bdd4a195fa66256bc0/src/starkware/cairo/common/cairo_blake2s/blake2s.cairo#L723

Reviewable status: 1 of 2 files reviewed, 1 unresolved discussion (waiting on @liorgold2)

crates/apollo_starknet_os_program/src/cairo/starkware/starknet/core/os/naive_blake.cairo line 6 at r2 (raw file):

No, we don't need RC here.
Did you try not to increment at all?

Reviewable status: 1 of 2 files reviewed, 1 unresolved discussion (waiting on @liorgold2)

crates/apollo_starknet_os_program/src/cairo/starkware/starknet/core/os/naive_blake.cairo line 6 at r2 (raw file):

Yes, that works if I add the RC. It fails without the flag, which confirms my theory as to why they had to increment ap

@Yoni-Starkware reviewed 1 of 2 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @liorgold2)

@liorgold2 reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @meship-starkware)